Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Remove legacy healthcheck files and structures #10542

Merged
merged 17 commits into from
Jul 8, 2022
Merged

Remove legacy healthcheck files and structures #10542

merged 17 commits into from
Jul 8, 2022

Conversation

frouioui
Copy link
Member

@frouioui frouioui commented Jun 17, 2022 •
edited
Loading

Description

This Pull Request removes the legacy healthcheck and the related code (legacy topo watcher, legacy replication lag, etc). The different usages have been replaced by the new implementation of the healthcheck. The different files hosting the legacy code have been removed.

Related Issue(s)

Checklist

  • "Backport me!" label has been added if this change should be backported
  • Tests were added or are not required
  • Documentation was added or is not required

@frouioui
Changed the txthrottler to use the new healthcheck implementation
499ff49 
Signed-off-by: Florent Poinsard <florent.poinsard@outlook.fr>
@frouioui frouioui added this to the v14.0 milestone Jun 17, 2022
@frouioui frouioui force-pushed the remove-legacy-healthcheck-struct branch from f0791e4 to b6fc805 Compare June 20, 2022 09:03
@frouioui
Removed the use of the legacy healthcheck in the tx_throttler
a809a13 
Signed-off-by: Florent Poinsard <florent.poinsard@outlook.fr>
@frouioui frouioui force-pushed the remove-legacy-healthcheck-struct branch from b6fc805 to a809a13 Compare June 20, 2022 09:08
@frouioui
Started the removal of the legacy healthcheck in vtworker
860c6fd 
Signed-off-by: Florent Poinsard <florent.poinsard@outlook.fr>
@frouioui
Remove the legacy healthcheck from vtworker
1b445b8 
Signed-off-by: Florent Poinsard <florent.poinsard@outlook.fr>
@frouioui
Remove the legacy healthcheck from vtworker
80cd193 
Signed-off-by: Florent Poinsard <florent.poinsard@outlook.fr>
@frouioui
Remove the legacy healthcheck from the wrangler
96932b0 
Signed-off-by: Florent Poinsard <florent.poinsard@outlook.fr>
@frouioui
Remove the legacy healthcheck and al. files
97400e0 
Signed-off-by: Florent Poinsard <florent.poinsard@outlook.fr>
@frouioui
Applied goimports and linter on the throttler package
5c66c6e 
Signed-off-by: Florent Poinsard <florent.poinsard@outlook.fr>
@frouioui
Fixed build issue and code comments
4654a77 
Signed-off-by: Florent Poinsard <florent.poinsard@outlook.fr>
@frouioui frouioui marked this pull request as ready for review June 21, 2022 16:01
@github-actions
Copy link
Contributor

github-actions bot commented Jun 21, 2022 •
edited by deepthi
Loading

Review Checklist

Hello reviewers! 👋 Please follow this checklist when reviewing this Pull Request.

General

  • Ensure that the Pull Request has a descriptive title.
  • If this is a change that users need to know about, please apply the release notes (needs details) label so that merging is blocked unless the summary release notes document is included.
  • If a new flag is being introduced, review whether it is really needed. The flag names should be clear and intuitive (as far as possible), and the flag's help should be descriptive.
  • If a workflow is added or modified, each items in Jobs should be named in order to mark it as required. If the workflow should be required, the GitHub Admin should be notified.

Bug fixes

  • There should be at least one unit or end-to-end test.
  • The Pull Request description should either include a link to an issue that describes the bug OR an actual description of the bug and how to reproduce, along with a description of the fix.

Non-trivial changes

  • There should be some code comments as to why things are implemented the way they are.

New/Existing features

  • Should be documented, either by modifying the existing documentation or creating new documentation.
  • New features should have a link to a feature request issue or an RFC that documents the use cases, corner cases and test cases.

Backward compatibility

  • Protobuf changes should be wire-compatible.
  • Changes to _vt tables and RPCs need to be backward compatible.
  • vtctl command output order should be stable and awk-able.

@deepthi
Copy link
Member

deepthi commented Jun 21, 2022

Since this is cleanup, we don't need to back port it to v14. It's a teeny bit risky to refactor this close to release on the release branch.

@frouioui frouioui modified the milestones: v14.0, v15.0 Jun 21, 2022
@frouioui
Fixed comment of GetTabletStats
7901bb7 
Signed-off-by: Florent Poinsard <florent.poinsard@outlook.fr>
@frouioui
Removed duplicated function GetTabletStats
2436bcc 
Signed-off-by: Florent Poinsard <florent.poinsard@outlook.fr>
systay
systay reviewed Jun 23, 2022
View reviewed changes
go/vt/worker/executor.go Outdated Show resolved Hide resolved
deepthi
deepthi reviewed Jun 28, 2022
View reviewed changes
go/vt/vtgate/vtgate.go Outdated Show resolved Hide resolved
go/vt/discovery/healthcheck.go Outdated Show resolved Hide resolved
go/vt/discovery/healthcheck.go Outdated
Comment on lines 173 to 174
TabletRecorder

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are we embedding an interface into another? Do we have implementations of TabletRecorder that don't implement HealthCheck?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we have implementations of TabletRecorder that don't implement HealthCheck?

No, we don't.

Why are we embedding an interface into another?

This was done so we can use the healthcheck (discovery.HealthCheck) as a tablet recorder in tx throttler:

vitess/go/vt/vttablet/tabletserver/txthrottler/tx_throttler.go

Line 199 in 4654a77

topologyWatcherFactory = func(topoServer *topo.Server, tr discovery.TabletRecorder, cell, keyspace, shard string, refreshInterval time.Duration, topoReadConcurrency int) TopologyWatcherInterface {

Ultimately, the topo watcher in the discovery package uses a TabletRecorder.

Previously, the LegacyHealthCheck interface was embedding LegacyTabletRecorder:

vitess/go/vt/discovery/legacy_healthcheck.go

Lines 285 to 289 in d037f24

type LegacyHealthCheck interface {
// LegacyTabletRecorder interface adds AddTablet and RemoveTablet methods.
// AddTablet adds the tablet, and starts health check on it.
// RemoveTablet removes the tablet, and stops its StreamHealth RPC.
LegacyTabletRecorder

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should not have to embed it for things to work.

go/vt/wrangler/keyspace.go Outdated Show resolved Hide resolved
go/vt/discovery/healthcheck.go Show resolved Hide resolved
go/vt/wrangler/keyspace.go Outdated Show resolved Hide resolved
go/vt/throttler/manager.go Outdated Show resolved Hide resolved
@frouioui
Merge remote-tracking branch 'upstream/main' into remove-legacy-healt…
c5cc2fa 
…hcheck-struct

Signed-off-by: Florent Poinsard <florent.poinsard@outlook.fr>
@frouioui
Reverted the changes to throttler.Result
60e6279 
Signed-off-by: Florent Poinsard <florent.poinsard@outlook.fr>
@frouioui
Use GetHealthyTabletStats in waitForDrainInCell and removed unrequire…
a93fda6 
…d target.shard checks

Signed-off-by: Florent Poinsard <florent.poinsard@outlook.fr>
@frouioui
Addition of a NewCellTabletsWatcher to waitForDrainInCell
e3aba1b 
Signed-off-by: Florent Poinsard <florent.poinsard@outlook.fr>
@frouioui frouioui requested a review from deepthi July 4, 2022 15:01
deepthi
deepthi approved these changes Jul 6, 2022
View reviewed changes
Copy link
Member

@deepthi deepthi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only one change is required - to remove TabletRecorder from HealthCheck interface.
Once that is done, this can be merged.
Nice work, and thank you for your patience during review.

@frouioui
Merge branch 'main' into remove-legacy-healthcheck-struct
25a59d4 
Signed-off-by: Florent Poinsard <florent.poinsard@outlook.fr>
@frouioui
Moved TabletRecorded methods inside the HealthCheck interface
b89882f 
Signed-off-by: Florent Poinsard <florent.poinsard@outlook.fr>
@frouioui frouioui merged commit da6f297 into vitessio:main Jul 8, 2022
@frouioui frouioui deleted the remove-legacy-healthcheck-struct branch July 8, 2022 15:22
@frouioui
Copy link
Member Author

frouioui commented Jul 8, 2022

Only one change is required - to remove TabletRecorder from HealthCheck interface.

We decided to remove the TabletRecorder interface and move its methods directly to the HealthCheck interface. They were both implemented by the same set of structures. This was done via b89882f.

@timvaillancourt timvaillancourt mentioned this pull request Jun 26, 2024
Merged
5 tasks
This was referenced Oct 2, 2024
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@systay systay systay left review comments

@deepthi deepthi deepthi approved these changes

@rohit-nayak-ps rohit-nayak-ps Awaiting requested review from rohit-nayak-ps rohit-nayak-ps is a code owner

@mattlord mattlord Awaiting requested review from mattlord mattlord is a code owner

@harshit-gangal harshit-gangal Awaiting requested review from harshit-gangal harshit-gangal is a code owner

@shlomi-noach shlomi-noach Awaiting requested review from shlomi-noach shlomi-noach is a code owner

@ajm188 ajm188 Awaiting requested review from ajm188 ajm188 is a code owner

@doeg doeg Awaiting requested review from doeg

@notfelineit notfelineit Awaiting requested review from notfelineit notfelineit is a code owner

Assignees
No one assigned
Labels
Component: Cluster management Type: Internal Cleanup
Projects
None yet
Milestone
v15.0
Development

Successfully merging this pull request may close these issues.

Delete legacy_healthcheck.go and any related structs that are no longer needed
3 participants
@frouioui @deepthi @systay